Skip to content

Deprecate ExprSchema functions #15847

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ajita-asthana
Copy link

@ajita-asthana ajita-asthana commented Apr 25, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the common Related to common crate label Apr 25, 2025
Copy link
Contributor

@m09526 m09526 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In deprecation note, state the fully qualified path to the replacement method, i.e. ExprSchemable::to_field.

@alamb
Copy link
Contributor

alamb commented May 7, 2025

Marking as draft as I think this PR is no longer waiting on feedback and I am trying to clear review queue. Please mark it as ready for review when it is ready for another look

@alamb alamb changed the title deprecate schema expressions Deprecate ExprSchemable functions May 9, 2025
@alamb alamb requested a review from timsaucer May 9, 2025 21:35
@ajita-asthana ajita-asthana requested a review from m09526 May 16, 2025 02:25
@@ -961,16 +961,19 @@ impl Display for DFSchema {
/// widely used in the DataFusion codebase.
pub trait ExprSchema: std::fmt::Debug {
/// Is this column reference nullable?
#[deprecated(since = "48.0.0", note = "Use ExprSchemable::nullable")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the note point the user to use field_from_column? This looks self referential.

fn nullable(&self, col: &Column) -> Result<bool> {
Ok(self.field_from_column(col)?.is_nullable())
}

/// What is the datatype of this column?
#[deprecated(since = "48.0.0", note = "Use ExprSchemable::data_type")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about field_from_column

fn data_type(&self, col: &Column) -> Result<&DataType> {
Ok(self.field_from_column(col)?.data_type())
}

/// Returns the column's optional metadata.
#[deprecated(since = "48.0.0", note = "Use ExprSchemable::metadata")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about field_from_column

@timsaucer timsaucer changed the title Deprecate ExprSchemable functions Deprecate ExprSchema functions May 16, 2025
@timsaucer
Copy link
Contributor

I also think the comment is pointing from ExprSchema -> ExprSchemable instead of recommending the method within the trait. I see that ExprSchemable does use calls to ExprSchema but this trait should be usable without the context of ExprSchemable I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate ExprSchema functions
5 participants